Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iQue] Match z_message #2435

Merged
merged 7 commits into from
Jan 22, 2025
Merged

[iQue] Match z_message #2435

merged 7 commits into from
Jan 22, 2025

Conversation

cadmic
Copy link
Contributor

@cadmic cadmic commented Jan 20, 2025

The big clue was that the debug string

"HI_SCORE( kanfont->mbuff.nes_mes_buf[message->rdp] & 0xff000000 ) = %x\n"

was changed to

"HI_SCORE( ((unsigned char*)(kanfont->mbuff.nes_mes_buf))[message->rdp] & 0xff000000 ) = %x\n"

so I think they did a find-and-replace everywhere to workaround the EGCS bug. I made macros for msgCtx->msgBufDecoded, msgCtx->msgBufDecodedWide, font->msgBuf, and font->msgBufWide which are kind of annoying to read but maybe not terrible

Copy link
Contributor

@AngheloAlf AngheloAlf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused as to why the new PRINTFs aren't fenced in a version ifdef.
They don't mess up the debug rom?

src/code/z_message.c Outdated Show resolved Hide resolved
src/code/z_message.c Outdated Show resolved Hide resolved
src/code/z_message.c Outdated Show resolved Hide resolved
@cadmic
Copy link
Contributor Author

cadmic commented Jan 20, 2025

The new PRINTFs are in NTSC-only code, so they wouldn't show up in the debug ROM. When matching NTSC we didn't bother to guess what they were (and some of them ended up being subtly different from PAL anyway)

src/code/z_message.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@fig02 fig02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the macros as array accesses look goofy THING(p)[i], but dont think theres anything that can really be done about it

@cadmic
Copy link
Contributor Author

cadmic commented Jan 21, 2025

we could also write THING[i], e.g. #define MSG_BUF_DECODED (msgCtx->msgBufDecoded). It's a bit more magical because it requires something named msgCtx in scope, but maybe it looks better?

@cadmic
Copy link
Contributor Author

cadmic commented Jan 21, 2025

Here's how that would look: cadmic@2d9686d

@fig02
Copy link
Collaborator

fig02 commented Jan 21, 2025

I like it alot more, but would understand if others are hesitant

@cadmic
Copy link
Contributor Author

cadmic commented Jan 21, 2025

I think I like it better too so I made the change

@fig02
Copy link
Collaborator

fig02 commented Jan 21, 2025

@AngheloAlf pinging incase you want to re-review with the new change

Copy link
Contributor

@AngheloAlf AngheloAlf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I slightly prefer the old syntax but it is fine either way

@fig02 fig02 removed the wait Wait to merge label Jan 21, 2025
@fig02 fig02 merged commit 06904e1 into zeldaret:main Jan 22, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants